Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggregate event logs for unsupported chains #1921

Merged
merged 16 commits into from
Sep 25, 2024
Merged

Conversation

hectorgomezv
Copy link
Member

@hectorgomezv hectorgomezv commented Sep 12, 2024

Summary

Derived from this previous work, this PR reduces the number of logs emitted while receiving events for unsupported chains.

Since this event amount could be high in some situations, the errors associated with the reception of these events are grouped by chainId and aggregated. A warning log is printed each minute for each affected chain, holding the count of events received associated with the chain.

Changes

  • Adds cached counter holding the occurrences of unsupported events for each chain.
  • Adds a timer that prints the occurrences summary each minute.
  • Adds the related tests.

@hectorgomezv hectorgomezv self-assigned this Sep 12, 2024
@coveralls
Copy link

coveralls commented Sep 12, 2024

Pull Request Test Coverage Report for Build 11035173943

Details

  • 27 of 27 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 90.809%

Totals Coverage Status
Change from base Build 11031898837: 0.08%
Covered Lines: 8375
Relevant Lines: 8851

💛 - Coveralls

Base automatically changed from chain-based-event-filtering to main September 13, 2024 12:34
private static readonly HOOK_TYPE = 'hook';
private static readonly UNSUPPORTED_CHAIN_EVENT = 'unsupported_chain_event';
public static readonly UNSUPPORTED_EVENTS_LOG_INTERVAL = 60 * 1000; // 1 minute
Copy link
Member Author

@hectorgomezv hectorgomezv Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add this to the configuration if we plan to change it dynamically. I opted to keep it here for now, but I'm open to externalizing the configuration.

@hectorgomezv hectorgomezv marked this pull request as ready for review September 13, 2024 18:14
@hectorgomezv hectorgomezv requested a review from a team as a code owner September 13, 2024 18:14
iamacook
iamacook previously approved these changes Sep 16, 2024
MemoizedFunction;
private readonly unsupportedEventsStore: Map<string, number> = new Map();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we have two instances and this is stored in memory, won't this mean the log is emitted twice each time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this to Redis in aea8544.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for looking into this Aaron! The memory storage was created to alleviate cache usage when receiving a lot of unsupported events in a short period. The log would indeed be emitted N times on each cycle (N being the number of running instances). Still, each instance would print the number of events received by the instance, and monitoring tools can aggregate the numbers if needed.

Sorry, this approach was highly opinionated and probably should have been discussed beforehand, I'm fine with using Redis for storing a unified counter, but I also think we should use an atomic counter (Redis INCR) instead of a plain hash (as implemented on aea8544) for both consistency (dirty writes) and performance. Wdyt @iamacook @PooyaRaki ?

(Btw, if we think this should be changed, I'd be super happy to tackle it on a separate PR so we can merge this one and unblock the feature 🙂 )

@iamacook iamacook self-requested a review September 17, 2024 07:49
@hectorgomezv hectorgomezv changed the base branch from main to add-cached-counters September 24, 2024 10:51
@hectorgomezv hectorgomezv marked this pull request as ready for review September 24, 2024 11:28
Add getCounter and setCounter to ICacheService
Base automatically changed from add-cached-counters to rename-cache-functions September 24, 2024 11:29
Base automatically changed from rename-cache-functions to main September 25, 2024 11:16
@hectorgomezv hectorgomezv dismissed iamacook’s stale review September 25, 2024 11:16

The base branch was changed.

@@ -10,7 +10,9 @@ import { EventNotificationsHelper } from '@/domain/hooks/helpers/event-notificat
import { EventCacheHelper } from '@/domain/hooks/helpers/event-cache.helper';

@Injectable()
export class HooksRepositoryWithNotifications implements IHooksRepository {
export class HooksRepositoryWithNotifications
implements IHooksRepository, OnModuleInit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to implement OnModuleInit here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just found out that onModuleInit lifecycle hook is called by NestJS if a class defines a method called onModuleInit, regardless of whether the class explicitly implements the OnModuleInit interface.

So even if the interface is not mandatory for the hook to work, I think it is a good practice to use it for clarity and to ensure your class conforms to the expected structure, so I've explicitly set both HooksRepositoryWithNotifications and HooksRepository as OnModuleInit implementations in 7d695ee

@hectorgomezv hectorgomezv merged commit 06a9378 into main Sep 25, 2024
18 checks passed
@hectorgomezv hectorgomezv deleted the aggregate-event-logs branch September 25, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants